fix: accept string node ids from the Files sidebar when requesting a signature - #8367

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

... and 221 files with indirect coverage changes

Thanks — agreed on all three points, and the trace changed my view of where the fix belongs. Below is the #8363 path on main, one row per place the value is read or transformed. The conclusion first: a numeric string in nodeId is a valid representation of a Nextcloud node by Nextcloud's own contract, the frontend serializer is the first place that loses it, and the backend would not accept it either. fileId/id never leave the number domain on this path, so they should not change.

Where the string comes from

resultToNode() in @nextcloud/files 4.0.0 (dist/dav.mjs:232) builds the node with id = props.fileid, where props come from the webdav 5.10 XML parser (fast-xml-parser). I checked the parser's behaviour with the exact options webdav uses (numberParseOptions: { hex: true, leadingZeros: false }):

Then, in Node itself: get fileid() returns a number only if _data.id is a number, otherwise undefined; get id() returns String(_data.id). So with snowflake IDs fileid is always undefined and id is always a string — the string is not an anomaly of one code path, it is the representation Nextcloud is moving to. (In the reporter's scenario the node comes from client.stat() after copyFile() in apps/files/src/actions/moveOrCopyAction.ts, which goes through the same resultToNode().)

The path

fileId and id on this path only ever come from LibreSign's own API (FileListService returns id/nodeId as int), so they stay in the number domain; nothing in the trace justifies accepting strings for them.

Where the fix belongs

Two places make the representation incorrect, and neither should convert with Number():

1. Frontend — serializeRequestFile() forwards nodeId as it is when it is a positive integer or a string of digits; fileId/id keep the number-only checks. AppFilesTab's FileInfo.id type becomes number | string to match tab.ts. Nothing else in the frontend path needs to change for this bug.
2. Backend — saveFile() and getNodeFromData() cast nodeId to int the way FileInputValidator already does (PHP int is 64-bit, so snowflake IDs are safe there), and the local $fileId in saveFile() is renamed to $nodeId. Without this, forwarding the string only moves the 422 from "parameter is required" to a TypeError message.

One decision I need from you — the API contract. LibresignNewFile.nodeId is declared non-negative-int (ResponseDefinitions.php:75, integer/int64 in openapi.json). Options:

• (a) declare nodeId?: non-negative-int|numeric-string and regenerate openapi.json and the TS types — consistent with the Nextcloud 33 note that APIs migrated to snowflake IDs use strings, and the frontend stops sending a value its own schema rejects;
• (b) keep integer in the spec and only tolerate the string server-side.

I lean to (a); it is the honest description of what the endpoint accepts, and it is one line plus generated files.

Tests (real sidebar data, no 'temp-node')

• tab.spec.ts: a real @nextcloud/files File with id: '9007199254740993' → enabled() → fileInfo.id is that string, untouched (fileid is undefined there).
• AppFilesTab.spec.ts: update({ id: '9007199254740993', name, path }) → addFile receives nodeId: '9007199254740993' unchanged.
• files.spec.ts: with that placeholder selected, saveOrUpdateSignatureRequest() sends file: { nodeId: '9007199254740993' }; a string fileId/id is still not accepted.
• RequestSignatureServiceTest: saveFile() with file.nodeId = '35523' calls getByNodeId(35523) — this branch has no coverage today, so I will add the int case as well.
• FileServiceTest: getNodeFromData() with file.nodeId = '35523' calls getFileByNodeId(35523) (the int case exists at l. 223).

Leftovers I will not touch here (for a separate cleanup issue, if you agree)

• src/actions/openInLibreSignAction.js:77 — files: nodes.map(node => ({ fileId: node.fileid ?? node.id })): a Nextcloud node id sent as fileId (the old model).
• AppFilesTab.vue:243 parseInt(rawNodeId, 10) and src/actions/showStatusInlineAction.js:12 — numeric conversions of node ids.
• lib/Service/SignFileService.php:143–144 — same raw nodeId → getByNodeId(int) pattern as saveFile().
• getFileIdByNodeId() strict === between a string node id and the API's int, and the placeholder key derived with unary minus — both lose meaning above MAX_SAFE_INTEGER.

If this reads right to you, I will rewrite the PR along these lines (it becomes frontend + backend; I will check the cherry-pick on stable32–stable35 before asking for backports).

Thanks, this trace is much clearer, and the proposed direction looks consistent with what I found as well.

I checked the flow against the current LibreSign code and the current Nextcloud contracts.

The important distinction seems to be:

• in the browser, @nextcloud/files exposes the current node identifier as Node.id: string | undefined;
• Node.fileid is the legacy numeric getter and is deprecated;
• in the Nextcloud PHP Files API, node IDs are still handled as int.

So the flow that makes the most sense for LibreSign is:

Nextcloud Node.id (string) -> LibreSign HTTP payload (string) -> validate/normalize at the PHP boundary -> Nextcloud OCP Files API (int)

This also means converting the value with Number() in JavaScript would be risky, because a valid 64-bit ID can be above Number.MAX_SAFE_INTEGER and lose precision before it reaches PHP.

I would also treat the fast-xml-parser behaviour as supporting context rather than the main reason for the change. It is part of the WebDAV dependency chain, not a LibreSign contract. The public @nextcloud/files Node.id contract already seems enough to define what LibreSign needs to support.

Your trace of the LibreSign path also looks correct to me:

• tab.ts can pass a string Nextcloud node ID;
• AppFilesTab currently declares FileInfo.id only as number, so that type looks inconsistent with what tab.ts can send;
• the serializer is the first place where the value is functionally lost;
• FileInputValidator already accepts a numeric string and converts it to int;
• later, RequestSignatureService::saveFile() and FileService::getNodeFromData() use the raw value again and can reach methods typed as int, so the backend path also needs to be aligned;
• nothing in this flow suggests that fileId or id should start accepting strings.

I think the typing is especially important here.

On the frontend, the type should reflect the real @nextcloud/files contract and keep nodeId in the string domain where required, instead of repeatedly coercing it with Number() or parseInt().

On the backend, I would prefer to validate and normalize the HTTP value once at a clear boundary, then let strong int typing carry that guarantee through the internal flow. If the types are correct, we should not need repeated (int) casts across different services.

The intended model would be:

HTTP nodeId: int | decimal string -> validate/normalize once -> internal nodeId: int -> OCP Files APIs

If the current data structure makes it difficult to propagate the normalized value, a small shared normalizer would be preferable to repeated casts in different methods.

Because of that, I think the safer frontend change would be to keep the fix specific to nodeId, instead of introducing one generic normalization helper for nodeId, fileId, and id.

For the API input, I think we can make the decision here: nodeId should accept both the existing integer representation and a decimal string. The string form is needed to safely carry the value from @nextcloud/files, while keeping integer input avoids breaking existing clients.

Could you also check what OpenAPI is actually generated from the proposed ResponseDefinitions change? I would prefer not to assume that numeric-string produces the schema we want. The important part is that the generated schema and TypeScript types clearly express integer | decimal-string.

Internally, PHP and database values can remain int / BIGINT, following the current Nextcloud server model.

I would keep changing nodeId in API responses from number to string out of this PR unless it turns out to be required to fix #8363. That is a broader compatibility question and should not block this regression fix.

The test plan looks good. Using a real node ID above Number.MAX_SAFE_INTEGER should protect the important part of this regression. It would also be useful to confirm that each method changed by the final fix keeps coverage for the existing integer path as well.

I also agree with leaving the other leftovers out of this PR. Since some old fileId names may only be naming leftovers while others may still represent an old data flow, tracing them individually before changing them seems safer.

For backports, I would check affectedness rather than only whether the patch cherry-picks cleanly.

stable34 is already confirmed by the reported bug. Since the string/snowflake node ID contract starts with Nextcloud 33, stable33 is also worth testing. I do not see a reason to include stable32 only because the patch applies cleanly.

With that, the direction of the rewrite looks good to me.

Rewritten along the lines you described — the description is updated with the new shape, the generated schema and the manual verification.

Short version of the points you asked me to check:

• Boundary: FileInputValidator::normalizeNodeId() runs once in RequestSignatureWorkflowService::createRequest() / updateExistingRequest() (for file and each files[]); nothing else casts. The nodeId?: int contract of saveFile() now holds from there on, and its local $fileId is renamed $nodeId.
• Frontend: the change is specific to nodeId and keeps the string; no Number() / parseInt() added. FileInfo.id in AppFilesTab is number | string, as tab.ts sends.
• Generated OpenAPI: non-negative-int|numeric-string becomes oneOf: [integer/int64/min 0, string] and nodeId?: number | string in TypeScript. openapi-extractor has no pattern support, so "digits only" is stated in the parameter description and enforced by the validator (422 as before for anything else). If you want a stricter schema I can hand-edit it, at the cost of losing it on the next composer openapi.
• Tests: each changed method has the string and the integer path, with 9007199254740993. Manual run with that value as a real fileid: before → 422 "File or files parameter is required"; after → 200 and node_id = 9007199254740993 in libresign_file (screenshots in the description).
• Backports: stable33 and stable34 only. The frontend commit cherry-picks cleanly on both; the backend one needs a manual patch there (the boundary is the controller and the validator is ValidateHelper), so I will open the two backports by hand after the merge.

One open point, in the description: LibresignNewFile is also the input of POST /file and POST /id-docs. I left those untouched (same behaviour as before for a string), it is the same one-line call at each boundary — here or in the leftovers issue, as you prefer.

Both points addressed (folded into the backend commit, rebased on main):

1. Other consumers of LibresignNewFile. The same normalizer now runs at every boundary that takes that payload: RequestSignatureWorkflowService (already), FileController::prepareFilesForSaving() for POST /file (with FileInputValidator injected directly rather than adding to the ValidateHelper façade that Remove the ValidateHelper compatibility facade #8357 removes) and IdDocsService::addIdDocs() / addFilesToDocumentFolder() for POST /id-docs (the rejection keeps the per-file JSON error with the file index, like the other validations there). So the schema means the same thing everywhere it is declared. Checked on the devcontainer: POST /file with "nodeId": "<digits>" → 200 and the node resolved; with "abc" → 422.

2. Strict boundary. normalizeNodeId() now either returns the payload with an int node id or throws the validator's existing Invalid fileID LibresignException: int passes, a string of digits within the int range becomes an int, and anything else present is rejected — leading zeros, sign, decimals, spaces, empty string, overflow, non-numeric strings, non-scalars (10 cases in FileInputValidatorTest). null/absent is left alone. It became an instance method for the translated message, so RequestSignatureWorkflowServiceTest now uses a real FileInputValidator and has a test that an invalid value is rejected before any service runs; IdDocsServiceTest covers the string → int path and the indexed error.

The description is updated accordingly.

Done (folded into the backend commit):

• normalizeNodeId() now enforces non-negative-int past the boundary: a negative int is rejected like the other invalid forms ('negative int' => [-1] added to the rejected cases; 0 and "0" added to the accepted ones, since the contract is non-negative). The int and the string paths now share the single is_int && >= 0 check.
• Wording: the accepted string is described as the canonical decimal string (digits only, no sign, no leading zeros, within the int range) in the docblock, in both controller parameter descriptions (and therefore in the regenerated OpenAPI/TypeScript) and in the PR description — "0042" is rejected on purpose.
• Backport section rewritten as you described: stable34 confirmed, branch-specific; stable33 verify affectedness first, then branch-specific if confirmed; stable32 not affected. No cherry-picks of this implementation.

The earlier CI failure was RequestSignatureControllerTest still building the workflow service with six arguments; it now uses a real FileInputValidator like the workflow test. CI is re-running.

/backport to stable35

/backport to stable34

/backport to stable33

Backports: stable35 #8395 already carries the workflow-boundary adaptation you added, nothing else needed from my side. For stable33 and stable34 the bot dropped the backend commit, so I opened #8397 and #8396 from my fork (I cannot push to the bot's branches): the bot's frontend commit plus a branch-specific RequestSignatureController::normalizeNodeId() with the same shape as your stable35 one, tests run on each branch. stable33 affectedness is confirmed and documented in #8397 (same @nextcloud/files 4.0.0 contract and the same raw path to getByNodeId(int)). stable32 untouched.